Skip to content

DRILL-6476: Generate explain plan which shows relation between Latera…#1308

Closed
HanumathRao wants to merge 2 commits into
apache:masterfrom
HanumathRao:DRILL-6476
Closed

DRILL-6476: Generate explain plan which shows relation between Latera…#1308
HanumathRao wants to merge 2 commits into
apache:masterfrom
HanumathRao:DRILL-6476

Conversation

@HanumathRao

Copy link
Copy Markdown
Contributor

…l and the corresponding Unnest.

@amansinha100 Please help review this PR.

This PR includes changes to the explain plan generation to generate a SrcOp: (majorfrag:minorFrag) for unnest operator so that it can be used for visual depiction of the relation.

Here is the plan which shows this relation.

explain plan for select * from (select customer.orders as c_orders from dfs./home/mapr/LATERAL/drill/exec/java-exec/src/test/resources/lateraljoin/nested-customer.parquet customer ) t1, lateral ( select t.ord.o_lineitems as items from unnest(t1.c_orders) t(ord) ) t2, lateral (select count(*) from unnest(t2.items) t3(item)) d1;

| 00-00 Screen : rowType = RecordType(ANY c_orders, ANY items, BIGINT EXPR$0): rowcount = 1.0, cumulative cost = {15.1 rows, 74.1 cpu, 0.0 io, 0.0 network, 0.0 memory}, id = 6223
00-01 Project(c_orders=[$0], items=[$1], EXPR$0=[$2]) : rowType = RecordType(ANY c_orders, ANY items, BIGINT EXPR$0): rowcount = 1.0, cumulative cost = {15.0 rows, 74.0 cpu, 0.0 io, 0.0 network, 0.0 memory}, id = 6222
00-02 Correlate(correlation=[$cor1], joinType=[inner], requiredColumns=[{1}]) : rowType = RecordType(ANY orders, ANY items, BIGINT EXPR$0): rowcount = 1.0, cumulative cost = {14.0 rows, 71.0 cpu, 0.0 io, 0.0 network, 0.0 memory}, id = 6221
00-04 Correlate(correlation=[$cor0], joinType=[inner], requiredColumns=[{0}]) : rowType = RecordType(ANY orders, ANY items): rowcount = 1.0, cumulative cost = {10.0 rows, 38.0 cpu, 0.0 io, 0.0 network, 0.0 memory}, id = 6218
00-07 Scan(groupscan=[ParquetGroupScan [entries=[ReadEntryWithPath [path=file:/home/mapr/LATERAL/drill/exec/java-exec/src/test/resources/lateraljoin/nested-customer.parquet]], selectionRoot=file:/home/mapr/LATERAL/drill/exec/java-exec/src/test/resources/lateraljoin/nested-customer.parquet, numFiles=1, numRowGroups=1, usedMetadataFile=false, columns=[orders]]]) : rowType = RecordType(ANY orders): rowcount = 4.0, cumulative cost = {4.0 rows, 4.0 cpu, 0.0 io, 0.0 network, 0.0 memory}, id = 6216
00-06 Project(items=[ITEM($0, 'o_lineitems')]) : rowType = RecordType(ANY items): rowcount = 1.0, cumulative cost = {2.0 rows, 2.0 cpu, 0.0 io, 0.0 network, 0.0 memory}, id = 6217
00-09 Unnest [SrcOp: (00-04)] : rowType = RecordType(ANY c_orders): rowcount = 1.0, cumulative cost = {1.0 rows, 1.0 cpu, 0.0 io, 0.0 network, 0.0 memory}, id = 6055
00-03 StreamAgg(group=[{}], EXPR$0=[COUNT()]) : rowType = RecordType(BIGINT EXPR$0): rowcount = 1.0, cumulative cost = {3.0 rows, 17.0 cpu, 0.0 io, 0.0 network, 0.0 memory}, id = 6220
00-05 Project($f0=[0]) : rowType = RecordType(INTEGER $f0): rowcount = 1.0, cumulative cost = {2.0 rows, 5.0 cpu, 0.0 io, 0.0 network, 0.0 memory}, id = 6219
00-08 Unnest [SrcOp: (00-02)]: rowType = RecordType(ANY items): rowcount = 1.0, cumulative cost = {1.0 rows, 1.0 cpu, 0.0 io, 0.0 network, 0.0 memory}, id = 6058

@kkhatua kkhatua left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested some formatting changes for compatibility with the existing Javascript patch

Prel parent = this.getRegisteredPrel(unnest.getParentClass());
if (parent != null && parent instanceof CorrelatePrel) {
OpId id = ids.get(parent);
return String.format(" [SrcOp: (%02d-%02d)] ", id.fragmentId, id.opId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to

 return String.format(" [srcOp=%02d-%02d] ", id.fragmentId, id.opId);

It makes it easier to use regex and extract the operator Id with JavaScript

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the change, the visual plan rendering remains unchanged:
image

With the change, the visual plan is rndered:
image

try (ClusterFixture cluster = builder.build();
ClientFixture client = cluster.clientFixture()) {
String explain = client.queryBuilder().sql(Sql).explainText();
String srcOp = explain.substring(explain.indexOf("SrcOp"));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the search term to match the syntax in my previous comment, so that this test doesn't fail.

if (!mq.isVisibleInExplain(rel, detailLevel)) {
// render children in place of this, at same level
explainInputs(inputs);
explainInputs(rel);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change will break the hash join explain if the inputs of HJ are swapped (based on line 68 above).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amansinha100 Thank you for the catch. I overlooked the above statement where it reverse populates the list for a hashjoin which is swapped. I will write a new explainInputs similar to that of correlatePrel and handle this specific case.

Are you fine in general with the approach apart from the above issue.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you already consider a potential alternative via the 'explainTerms()' API ? Some operators implement it..e.g SingleMergeExchangePrel implement it and the purpose is to customize what is shown in the Explain plan for that particular relnode. My thought was this could have been cleaner way by putting the same register/unregister logic you already have here into the CorrelatePrel.explainTerms(NumberingRelWriter) method. That way you don't have to check instanceof of certain specific operators in the NumberingRelWriter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amansinha100 I did consider about making changes to explainTerms, but it is not working for the two unnest nodes with same column name. Hence I went with this approach.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, in that case I suppose the different alias names are not accessible from within the unnest operator.

private final SqlExplainLevel detailLevel;
protected final Spacer spacer = new Spacer();
private final List<Pair<String, Object>> values = new ArrayList<>();
private final Map<String, Prel> registry;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The term 'registry' seems somewhat generic since this functionality is only used for the special case of Correlate-Unnest. Perhaps sourceOperatorRegistry or simply sourceOperatorMap ?

@HanumathRao

Copy link
Copy Markdown
Contributor Author

@kkhatua @amansinha100 Thank you for the review. I have made the required code changes. Please let me know if anything needs to be changed.

@amansinha100 amansinha100 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. +1

ilooner pushed a commit to ilooner/drill that referenced this pull request Jun 13, 2018
@ilooner ilooner closed this in 2427aa0 Jun 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants